The committers listed above are authorized under a signed CLA.

• ✅ login: lucavb / name: Luca Becker (f2b6229, 7a7fff8, 439120c)

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lucavb
Once this PR has been reviewed and has the lgtm label, please assign mstruebing for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Welcome @lucavb!

It looks like this is your first PR to kubernetes-client/javascript 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/javascript has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

If someone could share with me which forbidden keyword I've used in the main commit, then I'll gladly update it :)

I don't think this will address #2014. That issue was originally opened against the v0.x release line, but applies to the v1.x release line as well. The problem is that this library depends on openid-client, which has moved to ESM only. Won't that still be a problem for you?

I don't think this will address #2014. That issue was originally opened against the v0.x release line, but applies to the v1.x release line as well. The problem is that this library depends on openid-client, which has moved to ESM only. Won't that still be a problem for you?

In part? Yes. We still have to transform those packages. However, not having to transpile this package is already a big help. My test at least runs in a reasonable time < 2s where as before it was > 60s

Just my personal opinion - this module should not be published as CJS if it is known to be an incomplete solution because we will still get bug reports about it.

By the way, have you looked into require(esm) or dynamic import? require(esm) is on Node 22 and 23, and has been merged in Node 20 (so it should be in the next v20 release).

Just my personal opinion - this module should not be published as CJS if it is known to be an incomplete solution because we will still get bug reports about it.

I get that and I don't specifically like it either. I wish CJS would just go away and we'd all be happy. But I also feel like that is something we'll discuss in maybe 10 years time and unfortunately not today. And vitest similarly was also not a drop in replacement for us :/

With regards to the bug reports about it. I can kind of see that but then again you've also already received one issue about this package being ESM only.

By the way, have you looked into require(esm) or dynamic import? require(esm) is on Node 22 and 23, and has been merged in Node 20 (so it should be in the next v20 release).

I've just tested it (--experimental-require-module) and jest still complains. Which to be fair makes sense to me. They hook into the loading process and try to preprocess the file. So the problem is not that node in general can not deal with it but rather that Jest can't. Again really not happy about it.

So the problem is not that node in general can not deal with it but rather that Jest can't.

Jest's problems are very long standing and well known at this point. In my opinion, time would be better spent migrating away from Jest rather than shipping partially working CJS implementations to workaround Jest.

But I also feel like that is something we'll discuss in maybe 10 years time and unfortunately not today.

I think we are significantly closer than that, you're just using a test runner that actively fights against it 😄. require(esm) will be in all supported Node.js release lines in two months and dynamic import is already available.

Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages.

The list of commits with invalid commit messages:

• 7a7fff8 feat: add commonjs version in bundled package

Not exactly sure what else I can in turn offer here in terms of opinion. In my opinion offering CommonJS side by side does not really offer any downsides and would make consumers happy. Seemingly there is also another project that is currently not able to update to version 1.0.0 here.

So I guess my question is whether this addition has any chance of getting merged or not. And in case there is a chance that this will get merged, then I would need a hint in terms of what the problem with that one commit message is.

I'll defer to @brendandburns and @mstruebing on this one. I'm not in favor of shipping something that we know won't work.

this module should not be published as CJS if it is known to be an incomplete solution

I think that is the main point here and I agree with @cjihrig.
Would still be interesting what @brendanburns opinion about it is.

I wanted to add another data point from my side. In the latest version of Node 22, which we are currently using, --experimental-require-module is enabled by default, yet it still doesn't work as expected. Therefore, I'm not very optimistic that Node will resolve this issue for us anytime soon. Additionally, as mentioned earlier, migrating to Vitest is not a straightforward process for us.

I completely understand your decision to not continue supporting CommonJS and recognize that Jest is the main issue here. Given these challenges, if this package remains ESM-only, we might not be able to migrate to version 1.0.0 in the near future. The transpiling to CJS is proving to be too slow for our needs.

The transpiling to CJS is proving to be too slow for our needs.

At the risk of getting off topic here - why not vendor/fork and perform the transpilation once? This module doesn't get released all that frequently.

I agree with @cjihrig that we don't want to publish a commonjs module here. the older branches should provide sufficient utility while jest or node figures out how they want to fix this.

Apologies that this isn't a great solution for your case, but I don't want to confuse the support matrix by adding another option.

I am so confused by this project. If this library only supports ESM, then why don't any of the release notes say this? That's a massive breaking change and should be noted front and center. The README still shows every example with require() which is CommonJS and does not mention anywhere that this module is now ESM-only. Since this is typescript-based, and it is fairly common to transpile to both CommonJS and ESM, I assumed this library handled both. And since it wasn't mentioned anywhere prominent that the library is ESM-only, I just wasted hours working on an upgrade/migration only to hit an issue when I finally tried unit tests.

Could we at least update the README here to include this information? I'd think the examples in the README should be updated too, but at least a bold note about ESM vs CommonJS could save people a lot of wasted time. There are prime opportunities to do so in the first paragraph and also in the "Compatibility" section. But it isn't mentioned anywhere.

You're right, the README should be updated - this was missed while updating all of the examples. I have gone ahead and added bold text to the v1.0.0 release notes.

Thank you for considering the proposal. While I'm a bit disappointed by the decision not to support CommonJS, I respect your authority and the reasoning behind it. I appreciate the effort you put into maintaining and improving the project. I'll go ahead and close the PR now.

The biggest issue with simply ignoring CommonJS here is that it means users of CommonJS are also stuck with a version of this library which uses request instead of fetch and that has a massive number of vulnerabilities. This decision to favor the testing framework's capabilities over the needs of the client base is very disappointing because it leaves us stuck. We cannot readily transition our entire framework and infrastructure to ESM to support this library right now. We also do not want to be left with a highly vulnerable application due to the use of outdated libraries. So that means we may have to jettison the use if this library entirely.

I know this isn't really the fault of this library, since the entire way Node has opted to handle ESM/CommonJS support is atrocious. It is an "all or nothing" proposition with an incredibly fragile migration path. You cannot "phase in" the transition.

@robross0606 I totally understand the challenges of the transition. We're facing similar things in the Java client with the phasing out of java8.

It's also unfortunate that the request project decided to shut itself down despite lots of libraries (like the older version of this one) depending on it.

The challenge that we are faced with is that as various things we depend on switch to ESM, the set of functionality that we can import becomes smaller and smaller. I realize that by making the transition now we're effectively forcing the decision upstream. When we switched away from request, it made sense to try to combine the API change with the module change so that people only had a hard migration once.

I understand that for some users this is pretty painful.

That said, the beauty of open source is that you don't have to take a different dependency, as @cjihrig pointed out, you can fork this repository, apply this PR, and even push it to NPM if you want to, I expect that it shouldn't be too much work to do this every release and rebase as we add bug fixes etc.

It seems to me at least that ESM is the way of the future, so everyone is headed there eventually, so I don't see any long-term viable path to maintaining both libraries.

Thanks